Skip to content

Fix #11644: Fix broken dotty.tools.io.Path#parent #11662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 16, 2021

Conversation

griggt
Copy link
Contributor

@griggt griggt commented Mar 9, 2021

[test_non_bootstrapped]
[test_windows_full]

Fixes #11644

@griggt
Copy link
Contributor Author

griggt commented Mar 9, 2021

The failure in test_windows_full is unrelated to this PR.

else path match {
case "" | "." => Directory("..")
case _ => Directory(".")
}
case parent => Directory(parent)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed a case that is still broken because it takes this case parent path:

scala> Path("../..").parent
val res0: dotty.tools.io.Directory = ..      // should be ../../..

@@ -127,7 +127,15 @@ class Path private[io] (val jpath: JPath) {
* @return The path of the parent directory, or root if path is already root
*/
def parent: Directory = jpath.normalize.getParent match {
Copy link
Contributor Author

@griggt griggt Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it's best to avoid normalize altogether.

Consider this case:

  • working directory is /src/dotty
  • foo is a symlink to /tmp/foo

Before this PR:

scala> Path("foo/../bar").parent
val res0: dotty.tools.io.Directory = foo/../bar

scala> res0.jpath.toRealPath()
val res1: java.nio.file.Path = /tmp/bar

With this PR:

scala> Path("foo/../bar").parent
val res0: dotty.tools.io.Directory = .

scala> res0.jpath.toRealPath()
val res1: java.nio.file.Path = /src/dotty

Scala 2:

scala> Path("foo/../bar").parent
val res0: scala.reflect.io.Directory = foo/..

scala> res0.jfile.getCanonicalPath()
val res1: String = /tmp

readlink / realpath:

/src/dotty$ readlink -f foo/../bar/..
/tmp

/src/dotty$ realpath  foo/../bar/..
/tmp

/src/dotty$ realpath -L foo/../bar/..
/src/dotty

@michelou
Copy link
Contributor

michelou commented Mar 9, 2021

@griggt Thanks for fixing the files in directory project/scripts/.

@bishabosha bishabosha self-assigned this Mar 10, 2021
@bishabosha bishabosha self-requested a review March 10, 2021 09:19
@bishabosha
Copy link
Member

@griggt what else do you need to finish on this PR?

griggt added 2 commits March 15, 2021 20:18
Shell wildcards are not expanded when quoted, and thus the contents of
the temporary output directories were never being cleared.

One of the tests in bootstrapCmdTests had to be fixed as a result, since
it was relying on a TASTy file created by a previous test still existing
in the $OUT directory after having been cleared.
The previous implementation wrongly assumed that if
`jpath.normalize.getParent` returns null, then `jpath` must be a root.

However the null in this case really only means that no name elements
remain after normalization and the call to `getParent`. Since redundant
name elements such as "." and ".." may be removed by `normalize`, and
`getParent` may simply remove the last name element, there are cases
other than roots to consider, such as the current directory.

Some examples of broken behavior prior this commit:

  scala> Path("./foo.txt").parent
  val res0: dotty.tools.io.Directory = ./foo.txt  // should be Directory(.)

  scala> Path("foo.txt").parent
  val res1: dotty.tools.io.Directory = foo.txt    // should be Directory(.)

  scala> Path(".").parent
  val res4: dotty.tools.io.Directory = .          // should be Directory(..)

The changes here are based in part on the Scala 2.13 implementation of
scala.reflect.io.Path#parent

Fixes scala#11644
@griggt griggt marked this pull request as ready for review March 16, 2021 04:21
@griggt
Copy link
Contributor Author

griggt commented Mar 16, 2021

Several CI jobs (test_windows_fast, test_windows_full, scaladoc/build, scaladoc/community-docs) are failing due to a Bintray / JCenter outage: https://status.bintray.com/incidents/9n84kbm0bjh0

https://github.com/lampepfl/dotty/actions/runs/656300195
https://github.com/lampepfl/dotty/actions/runs/656300193

Calling java.nio.file.Path#normalize may result in resolving to a
different path than intended, as in the case where the given path
contains a `..` element and the preceding name is symbolic link.
Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bishabosha bishabosha merged commit 9567678 into scala:master Mar 16, 2021
@griggt griggt deleted the fix-#11644 branch March 16, 2021 15:14
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Could not find TASTY file" error when tasty/class files already exist
4 participants